Migrate avoid unnecessary return variable rule and its tests#237
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the avoid_unnecessary_return_variable lint rule to extend AnalysisRule and extracts the return variable usage logic into a dedicated ReturnVariableUsageVisitor. Additionally, the test suite has been migrated to a reflective test structure. The review feedback suggests extending SimpleAstVisitor instead of RecursiveAstVisitor in AvoidUnnecessaryReturnVariableVisitor to avoid redundant AST traversal, which also allows for the removal of a redundant super.visitReturnStatement(node) call.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| /// This visitor is searches all uses of a single local variable, | ||
| /// including variable declaration. | ||
| /// Visitor for [AvoidUnnecessaryReturnVariableRule]. | ||
| class AvoidUnnecessaryReturnVariableVisitor extends RecursiveAstVisitor<void> { |
There was a problem hiding this comment.
Since this visitor is registered via registry.addReturnStatement to process specific nodes, it does not need to recursively traverse the entire AST. Changing the base class to SimpleAstVisitor avoids redundant traversal and improves performance.
| class AvoidUnnecessaryReturnVariableVisitor extends RecursiveAstVisitor<void> { | |
| class AvoidUnnecessaryReturnVariableVisitor extends SimpleAstVisitor<void> { |
| rule.reportAtNode(node); | ||
|
|
||
| super.visitReturnStatement(node); |
There was a problem hiding this comment.
Calling super.visitReturnStatement(node) is redundant when using SimpleAstVisitor and registered via registry.addReturnStatement. Furthermore, it was applied inconsistently as it was skipped in several early return paths. Removing it simplifies the code and ensures consistent behavior.
| rule.reportAtNode(node); | |
| super.visitReturnStatement(node); | |
| rule.reportAtNode(node); |
danylo-safonov-solid
left a comment
There was a problem hiding this comment.
A few small suggestions, seems good otherwise
| lintName, | ||
| lintMessage, |
There was a problem hiding this comment.
I think we can inline those the same way as I proposed in #238
| if (expr is! SimpleIdentifier) { | ||
| return; | ||
| } | ||
|
|
||
| final ReturnStatement _returnStatement; | ||
| final element = expr.element; | ||
| if (element is! LocalVariableElement) { | ||
| return; | ||
| } | ||
|
|
||
| /// After visiting holds info about whether there are any tokens | ||
| /// between variable declaration and return statement | ||
| bool get foundTokensBetweenDeclarationAndReturn => | ||
| _foundTokensBetweenDeclarationAndReturn; | ||
| if (!element.isFinal && !element.isConst) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
nit - inconsistent braces. Since we are already omitting them in line 33, probably best to omit them throughout
final expr = node.expression;
if (expr is! SimpleIdentifier) return;
final element = expr.element;
if (element is! LocalVariableElement) return;
if (!element.isFinal && !element.isConst) return;
final block = node.parent;
if (block == null) return;| final tokenBeforeReturn = | ||
| _returnStatement.findPrevious(_returnStatement.beginToken); | ||
| bool _isSimpleIdentifierImmutable(SimpleIdentifier identifier) { | ||
| switch (identifier.element) { |
There was a problem hiding this comment.
Not 100% sure this will work
return switch (identifier.element) {
ClassElement _ => true,
final VariableElement variable ||
GetterElement(:final VariableElement variable) =>
variable.isFinal || variable.isConst,
_ => false
};but this should
return switch (identifier.element) {
ClassElement _ => true,
final VariableElement variable => variable.isFinal || variable.isConst,
GetterElement(:final PropertyInducingElement variable) =>
variable.isFinal || variable.isConst,
_ => false
};| /// VariableDeclarationStatement doesn't count when visiting SimpleIdentifier. | ||
| /// Any other amount of variable mentions implies that it is used somewhere | ||
| /// except return, so its existence is justified. | ||
| static const _badStatementCount = 1; |
There was a problem hiding this comment.
nit - I'd rather have static as the top-most in the class
| int _variableStatementCounter = 0; | ||
|
|
||
| /// Defines whether the variables is used in return statement only. | ||
| bool hasBadStatementCount() => |
There was a problem hiding this comment.
| bool hasBadStatementCount() => | |
| bool get hasBadStatementCount => |
2006202
into
analysis_server_migration
Migrated the
avoid_unnecessary_return_variablerule and its corresponding tests to comply with theanalyzerpackage.Changes
avoid_unnecessary_return_variableto use the currentanalyzersyntax.